Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: replaced ARCH with uname -m #100

Merged
merged 3 commits into from
Jan 5, 2024
Merged

fix: replaced ARCH with uname -m #100

merged 3 commits into from
Jan 5, 2024

Conversation

ShadowCurse
Copy link
Collaborator

@ShadowCurse ShadowCurse commented Jan 4, 2024 •
edited
Loading

Summary of the PR

In the previous commits/PR (#99, #98) the ARCH
env variable was introduced to the Dockerfile
and build_container.sh scripts. It was meant to
help with configuration of linux headers for
musl-tools, but did not work for some reason.
Considering the build_container.sh was already
using uname -m calls to get the current arch
this commit also uses it, so there is no need
for ARCH anymore.

Also removed && from the scripts and moved $(uname -m) to one global var.

For reviewers: don't approve before the CI is finished and it is clear that this PR fixes the issue.

Requirements

Before submitting your PR, please make sure you addressed the following
requirements:

  • All commits in this PR are signed (with git commit -s), and the commit
    message has max 60 characters for the summary and max 75 characters for each
    description line.
  • All added/changed functionality has a corresponding unit/integration
    test.
  • All added/changed public-facing functionality has entries in the "Upcoming
    Release" section of CHANGELOG.md (if no such section exists, please create one).
  • Any newly added unsafe code is properly documented.

@ShadowCurse
fix: replaced ARCH with uname -m
23c434c 
In the previous commits the ARCH
env variable was introduced to the Dockerfile
and build_container.sh scripts. It was meant to
help with configuration of linux headers for
musl-tools, but did not work for some reason.
Considering the build_container.sh was already
using `uname -m` calls to get the current arch
this commit also uses it, so there is no need
for ARCH anymore.

Signed-off-by: Egor Lazarchuk <[email protected]>
@stefano-garzarella
Copy link
Member

For reviewers: don't approve before the CI is finished and it is clear that this PR fixes the issue.

The CI seems finished, can you confirm that everything is fine?

What about failing the build if something is wrong?

@roypat
Copy link
Collaborator

roypat commented Jan 5, 2024

For reviewers: don't approve before the CI is finished and it is clear that this PR fixes the issue.

The CI seems finished, can you confirm that everything is fine?

What about failing the build if something is wrong?

I'm actually quite confused why it doesn't fail if something is wrong. The bash script has set -e, and the Dockerfile ends with RUN ..., so really it should propagate any errors 🤔

@ShadowCurse
Copy link
Collaborator Author

So as I can see the problematic step finished without errors: https://github.com/rust-vmm/rust-vmm-container/actions/runs/7412574298/job/20169661451?pr=100#step:8:1500

@stefano-garzarella
Copy link
Member

What about failing the build if something is wrong?

I'm actually quite confused why it doesn't fail if something is wrong. The bash script has set -e, and the Dockerfile ends with RUN ..., so really it should propagate any errors 🤔

This is a good point! Does anyone know why CI did not fail here, but executed the next command?

@roypat
Copy link
Collaborator

roypat commented Jan 5, 2024

What about failing the build if something is wrong?

I'm actually quite confused why it doesn't fail if something is wrong. The bash script has set -e, and the Dockerfile ends with RUN ..., so really it should propagate any errors 🤔

This is a good point! Does anyone know why CI did not fail here, but executed the next command?

Mhh, I think it has something to do with the &&. Testing locally, executing

#!/bin/bash

set -e

false

echo "hi"

prints nothing, while

#!/bin/bash

set -e

false && echo "hello"

echo "hi"

prints "hi". Bash 😖

@roypat
Copy link
Collaborator

roypat commented Jan 5, 2024

Given that we have set -e at the top of the script, would it make sense to avoid using && for chaining, and instead just split the commands across multiple lines always? That should fix it, I think.

@stefano-garzarella
Copy link
Member

Given that we have set -e at the top of the script, would it make sense to avoid using && for chaining, and instead just split the commands across multiple lines always? That should fix it, I think.

Yeah, I agree. Or we should add || exit 1, but I don't see the point while using set -e

@ShadowCurse
fix(build_container): removed &&
d8fddb6 
Removed && from the script because they
were causing the script build to succeed
even if errors were occurring.

Signed-off-by: Egor Lazarchuk <[email protected]>
@ShadowCurse
refactor(build_container): global ARCH variable
bcd5bc2 
There are several places that need current arch
of the system to install/configure the container.
This commit replaces separate calls of $(uname -m)
with one global ARCH variable.

Signed-off-by: Egor Lazarchuk <[email protected]>
@ShadowCurse
Copy link
Collaborator Author

Added couple of commits to remove && and move $(uname -m) to one global var.
Everything seems to work when I build locally.

@roypat
Copy link
Collaborator

roypat commented Jan 5, 2024

Added couple of commits to remove && and move $(uname -m) to one global var. Everything seems to work when I build locally.

Just to check that the build now fails if something in the script is wrong, can we try to remove the definition of ARCH and observe the build failing?

@ShadowCurse
Copy link
Collaborator Author

Without ARCH the output:

...
+ pushd /usr/include/-linux-musl
/opt/src/scripts/build_container.sh: line 21: pushd: /usr/include/-linux-musl: No such file or directory
The command '/bin/sh -c /opt/src/scripts/build_container.sh' returned a non-zero code: 1

@stefano-garzarella stefano-garzarella merged commit 30fa4e3 into rust-vmm:main Jan 5, 2024
2 checks passed
@ShadowCurse ShadowCurse deleted the new_image_4 branch January 5, 2024 14:23
ShadowCurse added a commit to ShadowCurse/rust-vmm-ci that referenced this pull request Jan 8, 2024
@ShadowCurse
update container version to v32
ffb4afc 
This version adds fixes needed for musl-tools
package to properly find linux headers.
More info in rust-vmm/rust-vmm-container#99
and rust-vmm/rust-vmm-container#100

Signed-off-by: Egor Lazarchuk <[email protected]>
ShadowCurse added a commit to ShadowCurse/rust-vmm-ci that referenced this pull request Jan 8, 2024
@ShadowCurse
update container version to v34
52aa267 
This version adds fixes needed for musl-tools
package to properly find linux headers.
More info in rust-vmm/rust-vmm-container#99
and rust-vmm/rust-vmm-container#100

Signed-off-by: Egor Lazarchuk <[email protected]>
@ShadowCurse ShadowCurse mentioned this pull request Jan 8, 2024
Merged
4 tasks
JonathanWoollett-Light pushed a commit to rust-vmm/rust-vmm-ci that referenced this pull request Jan 9, 2024
@ShadowCurse @JonathanWoollett-Light
update container version to v34
72ebeb3 
This version adds fixes needed for musl-tools
package to properly find linux headers.
More info in rust-vmm/rust-vmm-container#99
and rust-vmm/rust-vmm-container#100

Signed-off-by: Egor Lazarchuk <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@roypat roypat roypat approved these changes

@stefano-garzarella stefano-garzarella stefano-garzarella approved these changes

@andreeaflorescu andreeaflorescu Awaiting requested review from andreeaflorescu andreeaflorescu is a code owner

@lauralt lauralt Awaiting requested review from lauralt lauralt is a code owner

@stsquad stsquad Awaiting requested review from stsquad stsquad is a code owner

@JonathanWoollett-Light JonathanWoollett-Light Awaiting requested review from JonathanWoollett-Light

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ShadowCurse @stefano-garzarella @roypat